Skip to content

feat: add MFA authentication support to snowflake integration #2305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Oct 17, 2025
Merged

feat: add MFA authentication support to snowflake integration #2305

merged 10 commits into from
Oct 17, 2025

Conversation

ChinmayBansal
Copy link
Contributor

Related Issues

Proposed Changes:

This PR implements multi-factor authentication (MFA) support for the Snowflake
integration to address Snowflake's enhanced security requirements that mandate
MFA for connections. The implementation adds non-interactive authentication
methods while maintaining full backward compatibility.

Key Features Added:

  • Key-pair JWT Authentication (SNOWFLAKE_JWT) that supports:
    • Pre-configured private key file paths via private_key_file parameter
    • Private key passphrases via private_key_file_pwd parameter
    • JWT-based authentication without user interaction prompts
  • OAuth 2.0 Authentication (OAUTH) supporting:
    • Client Credentials flow with oauth_client_id and oauth_client_secret
    • Configurable token request URLs via oauth_token_request_url
    • Authorization URLs via oauth_authorization_url for future extensibility
  • Enhanced Security Features:
    • Credential masking in URI logging via _create_masked_uri() method
    • Secure handling of all sensitive data using Haystack's Secret class
    • Comprehensive parameter validation with clear error messages
  • Non-interactive Design: All authentication credentials sourced from
    environment variables, eliminating user prompts as specifically requested
  • Backward Compatibility: Existing password-based authentication (SNOWFLAKE)
    remains unchanged with no breaking changes

How did you test it?

  • Unit Tests: Added comprehensive test suite with 11 new test cases (36 total
    tests passing)
    • Code Quality: hatch run fmt and hatch run test:types both pass
    • Security Testing: Verified credential masking in logs
    • Manual verification: Tested all authentication methods and parameter
      validation

Notes for the reviewer

  • The core authentication logic is in _snowflake_uri_constructor() method
    (lines ~257-301)
    • Parameter validation logic is centralized in _validate_auth_params() method
      (lines ~152-185)
    • Security enhancement via _create_masked_uri() method (lines ~303-329) ensures
      sensitive data never appears in logs
    • The implementation specifically addresses the "no user authentication
      prompts" requirement

Checklist

@ChinmayBansal ChinmayBansal requested a review from a team as a code owner September 23, 2025 17:48
@ChinmayBansal ChinmayBansal requested review from sjrl and removed request for a team September 23, 2025 17:48
@github-actions github-actions bot added integration:snowflake type:documentation Improvements or additions to documentation labels Sep 23, 2025
@sjrl sjrl requested a review from medsriha October 6, 2025 08:29
medsriha
medsriha requested changes Oct 6, 2025
View reviewed changes
Copy link
Member

@medsriha medsriha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for your effort and contribution, @ChinmayBansal! I left a few comments, but overall this looks great. Would you mind moving the authentication code to a separate file outside of the main component to help keep things tidy? Also, were you able to test JWT and OAuth with real credentials?

@medsriha
Copy link
Member

@ChinmayBansal are you still working on this?

@ChinmayBansal
Copy link
Contributor Author

@medsriha Yes, I am. Will have updates shortly.

@ChinmayBansal
Copy link
Contributor Author

ChinmayBansal commented Oct 15, 2025 •
edited
Loading

@medsriha
I am currently trying to focus on JWT compatibility. The credentials are validate but the query execution encounters ADBC compatibility issues.

@ChinmayBansal ChinmayBansal changed the title add MFA authentication support to snowflake integration feat: add MFA authentication support to snowflake integration Oct 15, 2025
@ChinmayBansal
Copy link
Contributor Author

ChinmayBansal commented Oct 15, 2025 •
edited
Loading

Hey @medsriha,

I have addressed your feedback and tested my solution. I moved my solution to a different file: auth.py. I use the snowflake-connector-python directly. I tested with real Snowflake credentials and it worked.

The OAuth authentication implementation is complete but I was not able to test it because it requires accountadmin privileges which I do not have access to and it needs token exchange flow.

I think that JWT is Snowflake's recommended approach for MFA access.

Could you review?

@medsriha
Copy link
Member

Thank you @ChinmayBansal, I'll review this and try to test it with real credentials. Keep you posted

medsriha
medsriha requested changes Oct 15, 2025
View reviewed changes
Copy link
Member

@medsriha medsriha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChinmayBansal, nice work! I left you a few comments.

@ChinmayBansal
Copy link
Contributor Author

@medsriha, I have addressed your changes and also updated dependencies in the pyproject.toml file.

Could you review?

medsriha
medsriha requested changes Oct 16, 2025
View reviewed changes
Copy link
Member

@medsriha medsriha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ChinmayBansal — I left a few comments, nothing major. When you get a chance, could you also add some more unit tests? Current coverage is at 79%, and we aim to keep it closer to 90%. You can check coverage locally by running:

hatch -e test run all --cov=haystack_integrations tests/test_snowflake_table_retriever.py

@ChinmayBansal
Copy link
Contributor Author

@medsriha I have added more unit test cases and the coverage is now 93-94%. I also created a test_auth file to make the code a little bit more maintainable.

Let me know if more changes are needed.

medsriha
medsriha approved these changes Oct 16, 2025
View reviewed changes
Copy link
Member

@medsriha medsriha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💯

@medsriha medsriha merged commit 1867cdd into deepset-ai:main Oct 17, 2025
11 checks passed
@sjrl sjrl mentioned this pull request Oct 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@medsriha medsriha medsriha approved these changes

@sjrl sjrl Awaiting requested review from sjrl sjrl is a code owner automatically assigned from deepset-ai/open-source-engineering

Assignees

No one assigned

Labels

integration:snowflake type:documentation Improvements or additions to documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

update: update Snowflake integration to support MFA

2 participants

@ChinmayBansal @medsriha